Don't overwrite label, width, or sas.format in xportr_type()
#85
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes issue #75 by retaining column attributes (such as label). Tested with `xportr_label()` only, not with `metacore::set_variable_labels()` (is that even a real function?)
Now that `xportr_type()` is not overwriting column labels, applying it before or after `xportr_label()` should make no difference
In general, setting the type for each column should retain each column's attributes, such as label, width, and sas.format. These are set by other xportr functions. However, in cases where a column in the input dataset has a class already, such as a Date or a factor, this attribute should be dropped from the column. If it is not, it causes an error
Codecov Report
@@ Coverage Diff @@
## devel #85 +/- ##
==========================================
+ Coverage 66.00% 74.93% +8.93%
==========================================
Files 10 10
Lines 350 375 +25
==========================================
+ Hits 231 281 +50
+ Misses 119 94 -25
... and 4 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@EeethB can you have this go into the devel branch. @elimillera can you make it so that branches that are merged into devel are deleted ? |
bms63
reviewed
Apr 14, 2023
bms63
approved these changes
Apr 14, 2023
|
Comments have been added to NEWS.md |
|
@elimillera can you just once over this as well please?? |
elimillera
approved these changes
Apr 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #75
The issue here was that coercing columns with
as.character()oras.numeric()strips all attributes, not just the class attribute that we want to be rid of. I made a change to save attributes besides class (such as width, label, and format.sas), then restore those classes after coercion. I also wrote a test that checks for identical results whetherxportr_type()is called at the beginning or end of a pipeline.The function
metacore::set_variable_labels()was also referenced in the original issue. I cannot find such a function in metacore, so I have not tested against it here. If it sets labels with an attribute like xportr does, this will fix it as well.Should I ask the original issue filer to install this version for testing?
Please let me know if there's anything you want changed or included in this PR!